Skip to content

Conversation

@joaosaffran
Copy link
Collaborator

@joaosaffran joaosaffran commented Oct 24, 2025

This PR adds isfinite, isinf, isnan intrinsic test to long vectors.

Closes: #7850

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@joaosaffran joaosaffran marked this pull request as ready for review October 27, 2025 19:16
@joaosaffran joaosaffran requested a review from alsepkow October 27, 2025 19:21
INPUT_SET(InputSet::Positive, 1.0f, 1.0f, 65535.0f, 0.01f, 5531.0f, 0.01f, 1.0f,
0.01f, 331.2330f, 3250.01f);
INPUT_SET(InputSet::SelectCond, 0.0f, 1.0f);
INPUT_SET(InputSet::Infinite, std::numeric_limits<float>::infinity(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want to add some finite values to the set of values we test. Otherwise, all the results are just going to be false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward a bit, we know that we're going to be adding some more special values? Would it be better to have an input set that contains a mixture of non-special and special float values?

INPUT_SET(InputSet::Positive, 1.0, 1.0, 342.0, 0.01, 5531.0, 0.01, 1.0, 0.01,
331.2330, 3250.01);
INPUT_SET(InputSet::SelectCond, 0.0, 1.0);
INPUT_SET(InputSet::Infinite, std::numeric_limits<HLSLHalf_t>::infinity(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should double check what's happening here. Passing HLSLHalf_t as the template argument to std::numeric_limits will be doing an implicit conversion to float. Which is fine. And it looks like DirectX::PackedVector::XMConvertFloatToHalf does the right thing by converting that to an inf 'HALF'.

I think it would be better to do std::numeric_limits::infinity() to be more explicit. What do you think?

// Float Ops
//

#define BOOLEAN_FLOAT_OP(OP, IMPL) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming this macro will be helpful for the other float ops we're adding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"boolean float op" sounds a bit contradictory.

I think we're doing a bunch of "float specials" here, I wonder if it would make sense to group them in that way?

Copy link
Contributor

@alsepkow alsepkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor suggestions. Outside of those LGTM

@damyanp
Copy link
Member

damyanp commented Oct 28, 2025

Please associate this with the appropriate workitem (maybe with a "closes #xxx" type thing in the PR description)

@joaosaffran joaosaffran linked an issue Oct 28, 2025 that may be closed by this pull request
OP(Ternary, Select, 3, "TestSelect", "", " -DFUNC_TEST_SELECT=1",
"LongVectorOp", SelectCond, Default2, Default3)

OP(Unary, IsFinite, 1, "TestIsFinite", "", " -DFUNC_TEST_ISFINITE=1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Unary the right group to use here? Why not UnaryMath (and if so, should move this up next to the other unary math ops)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested to use 'FloatSpecial' as I don't think this is really a 'Math' Op either. 'UnaryLogical' would also fit IMO. But 'FloatSpecial' seems even better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(no need to change anything, calling it FloatSpecial is fine, but I can't resist responding....)

I don't think this is really a 'Math' Op either

The C implementations of these are all provided by math.h, and the C++ ones are in cmath....so, they're usually grouped in as math functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poor choice of wording on my part. It wasn't that it's not a math op and more so that Math op felt too generic for this context. The trig functions are technically unary math ops as well but grouping them as trig ops felt logical.

Just sharing my thoughts there!

INPUT_SET(InputSet::Positive, 1.0f, 1.0f, 65535.0f, 0.01f, 5531.0f, 0.01f, 1.0f,
0.01f, 331.2330f, 3250.01f);
INPUT_SET(InputSet::SelectCond, 0.0f, 1.0f);
INPUT_SET(InputSet::Infinite, std::numeric_limits<float>::infinity(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking forward a bit, we know that we're going to be adding some more special values? Would it be better to have an input set that contains a mixture of non-special and special float values?

// Float Ops
//

#define BOOLEAN_FLOAT_OP(OP, IMPL) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"boolean float op" sounds a bit contradictory.

I think we're doing a bunch of "float specials" here, I wonder if it would make sense to group them in that way?

Comment on lines 1496 to 1497
HLK_TEST(IsFinite, double);
// Explicit Cast
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
HLK_TEST(IsFinite, double);
// Explicit Cast
HLK_TEST(IsFinite, double);
// Explicit Cast

}
#endif
#ifdef FUNC_TEST_ISFINITE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this really be something like "FLOAT_SPECIALS" and have another define choose between isinifite, isinf and isnan?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was checking LongVectorOps.def, no other test requires 2 defines to specify the test. In some cases, like IS_REDUCTION_OP it modifies other parts of the code instead of defining functions to test. So I am not really sure if we should modify this for those specific tests. Unless the FLOAT_SPECIALS define modify some other parts of the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these IS_UNARY_OP with FUNC being one of isinfinite, isinf and isnan, and OUT_TYPE being bool?

Copy link
Contributor

@alsepkow alsepkow Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't actually need to define new helper functions in the HLSL source for these cases.
The 4th argument to the 'OP' macro in LongVectorOps.def is 'INTRINSIC'. Currently you're setting it to 'TestIsFinite', 'TestIsInf', or 'TestIsNan'.

Instead, you can put the actual HLSL intrinsic name there and the generic path in this test xml will handle it for you!
The only other thing to worry about with that is setting OUT_TYPE to bool, which you're already doing implicitly via the return type via the operator overload in your FLOAT_SPECIAL_OP macro :).

TLDR; Get rid of these function definitions and update the string in the 'OP' macros in LongVectorOps.def to be the actual intrinsic name.

Edit: You can actually swap to using the 'OP_DEFAULT' macro because you wont need custom defines anymore.

@joaosaffran joaosaffran marked this pull request as draft October 28, 2025 17:37
@joaosaffran joaosaffran marked this pull request as ready for review October 29, 2025 20:12
@joaosaffran joaosaffran changed the title Adding IsFinite long vector tests Adding IsFinite, IsInf and IsNan long vector tests Oct 29, 2025
}
#endif
#ifdef FUNC_TEST_ISFINITE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these IS_UNARY_OP with FUNC being one of isinfinite, isinf and isnan, and OUT_TYPE being bool?

OP_DEFAULT(UnaryMath, Log2, 1, "log2", "")
OP_DEFAULT_DEFINES(UnaryMath, Frexp, 1, "TestFrexp", "", " -DFUNC_FREXP=1")

OP(UnaryMath, IsFinite, 1, "TestIsFinite", "", " -DFUNC_TEST_ISFINITE=1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: These aren't really 'UnaryMath' operations. The name isn't currently used for anything outside of a human hint.
But I'm thinking 'FloatSpecial' would be better. My main nit is that these are logical operations, not math operations.

HLK_TEST(Frexp, float);
HLK_TEST(Abs, double);
HLK_TEST(Sign, double);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Float Special

HLK_TEST(IsInf, float);

HLK_TEST(IsNan, HLSLHalf_t);
HLK_TEST(IsNan, float);
Copy link
Contributor

@alsepkow alsepkow Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The other test cases are organized in groups by input type

Suggested change
HLK_TEST(IsNan, float);
HLK_TEST(IsInf, HLSLHalf_t);
HLK_TEST(IsFinite, HLSLHalf_t);
HLK_TEST(IsNan, HLSLHalf_t);
HLK_TEST(IsInf, float);
HLK_TEST(IsFinite, float);
HLK_TEST(IsNan, float);

INPUT_SET(InputSet::Positive, 1.0, 1.0, 342.0, 0.01, 5531.0, 0.01, 1.0, 0.01,
331.2330, 3250.01);
INPUT_SET(InputSet::SelectCond, 0.0, 1.0);
// HLSLHalf_t cast this to float when assigning it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// HLSLHalf_t cast this to float when assigning it.
// HLSLHalf_t has a constructor which accepts a float and converts it to half precision by
// clamping to the representable range via DirectX::PackedVector::XMConvertFloatToHalf.

Copy link
Contributor

@alsepkow alsepkow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your help!

@joaosaffran joaosaffran merged commit b3619bf into microsoft:main Oct 30, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in HLSL Roadmap Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Implement tests for isfinite, isinf and isnan Long Vector Execution Tests: Float Specific Ops

4 participants